Databases tree view #245

danvergara · 2024-11-15T20:25:30Z

Add a view to see databases and their tables

Description

The vast majority of database client software, if not all, show a list of databases and their tables as a tree structure. This is convenient for users that want to see all the database the have access to.
When it comes to dblab, things used to be a bit different, because database parameter was always required, either as a cli flag value or as part of the DSN. This was this way for the longest time because one reason. First off, dblab was heavily inspired by pgweb. This was the client I used to use at work, but suddenly I got tasked to manage a couple of microservices tied to MySQL databases, and I needed a FOSS alternative for Linux that worked as well as pgweb for my use case. pgweb rquires a database to connect to and only display its tables as a list. Then I tried to mimic a pgweb like client in the terminal and that's how dblab started. But things have changed I my needs have evolved and sometimes I need to look at other databases as well.

This MR basically removes the mandatory status of the database parameter and makes it optional. If the database is not provided, then, a tree structure is displayed showig databases and their tables.

Another important change has been made and it was removing the pagination. I have rarely found myself using it, because it's faster to make query to where I need to be.

Fixes #209
Fixes #232

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Tested it against pagila databse and the testing database of the project

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
I have checked my code and corrected any misspellings

…tory

…h databaseQuerier interface

ccoVeille · 2024-11-16T10:28:34Z

pkg/client/client.go

+	var (
+		resultSet = [][]string{}
+		db        *sqlx.DB
+	)
+
+	if c.activeDatabase != "" {
+		switch c.driver {
+		case drivers.Postgres, drivers.PostgreSQL, drivers.MySQL:
+			db = c.dbs[c.activeDatabase]
+		}
+	} else {
+		db = c.db
+	}


What about this?

Suggested change

var (

resultSet = [][]string{}

db *sqlx.DB

)

if c.activeDatabase != "" {

switch c.driver {

case drivers.Postgres, drivers.PostgreSQL, drivers.MySQL:

db = c.dbs[c.activeDatabase]

}

} else {

db = c.db

}

var (

resultSet = [][]string{}

)

db = c.db

if c.activeDatabase != "" {

switch c.driver {

case drivers.Postgres, drivers.PostgreSQL, drivers.MySQL:

db = c.dbs[c.activeDatabase]

}

}

This is so nice and elegant. Good catch!

ccoVeille · 2024-11-16T10:30:38Z

pkg/client/client.go

+	if c.activeDatabase != "" {
+		switch c.driver {
+		case drivers.Postgres, drivers.PostgreSQL, drivers.MySQL:
+			db = c.dbs[c.activeDatabase]


What about this?

Suggested change

db = c.dbs[c.activeDatabase]

db, ok = c.dbs[c.activeDatabase]

if !ok {

return nil, nil, return nil, fmt.Errorf("connection with %s database not found", c.activeDatabase)

}

I'm afraid about possible panic, when calling db.Query

Yeah, good catch there!

I'll add this.

Thanks!

ccoVeille · 2024-11-16T10:39:17Z

pkg/client/client.go

-func (c *Client) PreviousPage() (*Table, int, error) {
-	if err := c.paginationManager.PreviousPage(); err != nil {
-		return nil, 0, err
+		tables = append(tables, table)
 	}


Each row.Nect loop must be followed by checking rows.Err()

Suggested change

}

}

if err:= rows.Err(); err != nil {

return nil, err

}

ccoVeille · 2024-11-16T10:40:57Z

pkg/client/client.go

-	return c.db
-}
+		databases = append(databases, database)
+	}


Same issue about missing rows.Err check

ccoVeille · 2024-11-16T10:44:29Z

pkg/client/client.go

+	if driver == drivers.MySQL {
+		newConnString = strings.Replace(connString, "/", fmt.Sprintf("/%s", database), 1)
+	} else {
+		u, err := url.Parse(connString)
+		if err != nil {
+			return nil, err
+		}
+
+		u.Path = "/" + database
+		newConnString = u.String()
+	}


I would use a switch and return an error when it's none of the supported database types

I'm a bit worried by the fact you will apply the postgres way to any future database type that might be added

I agree with using a switch here, but the driver validation is done somewhere. If the execution reaches this point, the driver is actually valid. Regarding applying the postgres way, yeah a switch statement might help supporting a new use case.

ccoVeille · 2024-11-16T10:44:57Z

pkg/client/client.go

+	db, err := sqlx.Open(driver, newConnString)
+	if err != nil {
+
+		panic(err)


Why a panic?

Suggested change

panic(err)

return nil, err

I left it there by accident. I'll remove it,
Thanks!

ccoVeille · 2024-11-16T10:46:25Z

pkg/client/client_test.go

@@ -39,7 +39,15 @@ func TestMain(m *testing.M) {
 func generateURL(driver string) string {
 	switch driver {
 	case drivers.Postgres:


Why postgresql const is not present here? Isn't it tested?

Because the that constant is the one used at the time to test on CI. I can only use one at a time there.

ccoVeille · 2024-11-16T10:47:23Z

pkg/client/client_test.go

 	assert.Greater(t, len(r), 0)
 	assert.Greater(t, len(co), 0)


Suggested change

assert.Greater(t, len(r), 0)

assert.Greater(t, len(co), 0)

assert.NotEmpty(t, r)

assert.NotEmpty(t, co)

ccoVeille · 2024-11-16T10:47:48Z

pkg/client/client_test.go

+	assert.Greater(t, len(r), 0)
+	assert.Greater(t, len(co), 0)


Same you could use NotEmpty

ccoVeille · 2024-11-16T10:48:58Z

pkg/tui/tui.go

+			// if i == 0 {
+			// 	t.aw.content.SetCell(i+1, j, &tview.TableCell{Text: sc, Color: tcell.ColorRed})
+			// } else {
+			// }
+			t.aw.content.SetCellSimple(i+1, j, sc)


Why leaving commented out code?

Good catch!

I was testing out new colors.

danvergara · 2024-11-17T05:34:38Z

Hey @ccoVeille!

What a good code review there!

I already addressed all your comments. Let me know if I missed something.

Thanks so much!

P.S. Can I include you as a reviewer from this point on?

ccoVeille · 2024-11-17T05:52:00Z

Hey @ccoVeille!

What a good code review there!

Thanks, you are welcome

I already addressed all your comments. Let me know if I missed something.

Thanks so much!

P.S. Can I include you as a reviewer from this point on?

Sure

ccoVeille · 2024-11-17T06:06:19Z

Maybe you could add a co-authored-by tag

You can find my email here

samber/lo@52a73c3

So, just add this to the commit message of the changes you made with my review

Co-authored-by: ccoVeille <[email protected]>

ccoVeille · 2024-11-17T06:10:33Z

pkg/client/client.go

+	default:
+		u, err := url.Parse(connString)
+		if err != nil {
+			return nil, err
+		}
+
+		u.Path = "/" + database
+		newConnString = u.String()


This would be safer (I might have forgotten some)

Suggested change

default:

u, err := url.Parse(connString)

if err != nil {

return nil, err

}

u.Path = "/" + database

newConnString = u.String()

case drivers.PostgreSQL, drivers.Postgres:

u, err := url.Parse(connString)

if err != nil {

return nil, err

}

u.Path = "/" + database

newConnString = u.String()

default:

return nil, fmt.Errorf("unsupported driver %s", driver)

I disagree with this, since the driver validation is done somewhere else and the MySQL URL parsing is the corner case here The default behavior is parsing the DSN with url.Parse.

Ah OK then. I misunderstood

ccoVeille · 2024-11-17T06:13:13Z

pkg/client/client_test.go

 	assert.Greater(t, len(m.Indexes.Rows), 0)
 	assert.Greater(t, len(m.Indexes.Columns), 0)


Use NotEmpty

testifylint linter could help you a lot with such "issue"

It would also detect when you should use require.NoError

https://github.com/Antonboom/testifylint

It can be added easily to golangci-lint, bit you don't have golangci-lint yet,
maybe

I can help you here if you want

ccoVeille · 2024-11-17T06:18:26Z

pkg/client/client_test.go

+	c, err := New(opts)
+	assert.NoError(t, err)

 	tables, err := c.ShowTables()


here you should (must) use

Suggested change

c, err := New(opts)

assert.NoError(t, err)

tables, err := c.ShowTables()

c, err := New(opts)

require.NoError(t, err)

tables, err := c.ShowTables()

require is another package available in testify

The reason is that if an error is returned by New, c will be nil, so c.ShowTables will panic

Test should never panic

Ok, let's focus on the scope of the PR. We can address the test suite in another PR and issue. This test suite is not new, it's been 3 years since I set it up. I'm willing to improve it but later.

ccoVeille · 2024-11-17T06:27:05Z

pkg/client/client_test.go

@@ -212,23 +221,20 @@ func TestTableStructure(t *testing.T) {
 		Pass:   password,
 		Host:   host,
 		Port:   port,
-		DBName: name,
 		Schema: schema,
 		SSL:    "disable",
 		Limit:  100,
 	}

 	c, _ := New(opts)


You shouldn't ignore the error here

ccoVeille · 2024-11-17T06:27:30Z

pkg/client/client_test.go

@@ -247,17 +253,14 @@ func TestConstraints(t *testing.T) {

 	c, _ := New(opts)


ccoVeille · 2024-11-17T06:29:06Z

pkg/client/client_test.go

+	t.Logf("constraints columns %v", co)
+	t.Logf("constraints content %v", r)


These logs are not needed. If the test fail, you will get the info. If it doesn't fail, I don't see a need to see these lines when running a go test

Let's create an issue to improve the test suite.

…[email protected]>

danvergara · 2024-11-18T23:22:11Z

Maybe you could add a co-authored-by tag

You can find my email here

samber/lo@52a73c3

So, just add this to the commit message of the changes you made with my review
Co-authored-by: ccoVeille <[email protected]>

Alright @ccoVeille, I just added the co-authored tag. Honor when honor is due.

Can you go over the PR again, please? I 'd like to land this change tonight.

Arturomtz8

nice work man

danvergara added 4 commits November 15, 2024 14:16

feat(client): adapt the client to handler database-less connections

fbdef61

feat(connection): add the change to make the database param non-manda…

09b3fa6

…tory

feat(client): update the database implementations to be complaint wit…

f1b1495

…h databaseQuerier interface

feat(tui): refactor the tui to handler connection without the database

dcb4c84

danvergara self-assigned this Nov 16, 2024

danvergara marked this pull request as ready for review November 16, 2024 06:05

danvergara requested review from rkgarcia, Arturomtz8 and ThiagoMowszet November 16, 2024 06:05

ccoVeille reviewed Nov 16, 2024

View reviewed changes

danvergara requested a review from ccoVeille November 17, 2024 05:34

ccoVeille reviewed Nov 17, 2024

View reviewed changes

fix(pkg): address ccoVeille comments Co-authored-by: ccoVeille <38758…

e64fa29

…[email protected]>

danvergara force-pushed the databases-tree-view branch from 2f01c7e to e64fa29 Compare November 18, 2024 23:19

ccoVeille approved these changes Nov 18, 2024

View reviewed changes

Arturomtz8 approved these changes Nov 19, 2024

View reviewed changes

docs(readme): add instructions on how to use new tree view

d13d7fa

danvergara merged commit a0a6fdc into main Nov 19, 2024
8 checks passed

danvergara deleted the databases-tree-view branch November 19, 2024 06:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Databases tree view #245

Databases tree view #245

danvergara commented Nov 15, 2024 •

edited

Loading

ccoVeille Nov 16, 2024

danvergara Nov 17, 2024

ccoVeille Nov 16, 2024

danvergara Nov 17, 2024

ccoVeille Nov 16, 2024

ccoVeille Nov 16, 2024

ccoVeille Nov 16, 2024

danvergara Nov 17, 2024

ccoVeille Nov 16, 2024

danvergara Nov 17, 2024

ccoVeille Nov 16, 2024

danvergara Nov 17, 2024

ccoVeille Nov 16, 2024 •

edited

Loading

ccoVeille Nov 16, 2024

ccoVeille Nov 16, 2024

danvergara Nov 17, 2024

danvergara commented Nov 17, 2024

ccoVeille commented Nov 17, 2024

ccoVeille commented Nov 17, 2024

ccoVeille Nov 17, 2024

danvergara Nov 17, 2024

ccoVeille Nov 17, 2024

ccoVeille Nov 17, 2024

ccoVeille Nov 17, 2024

ccoVeille Nov 17, 2024

danvergara Nov 17, 2024 •

edited

Loading

ccoVeille Nov 17, 2024

ccoVeille Nov 17, 2024

ccoVeille Nov 17, 2024

danvergara Nov 17, 2024

danvergara commented Nov 18, 2024

Arturomtz8 left a comment

-			db = c.dbs[c.activeDatabase]
+			db, ok = c.dbs[c.activeDatabase]
+			if !ok {
+	 			return nil, nil, return nil, fmt.Errorf("connection with %s database not found", c.activeDatabase)
+			}

		assert.Greater(t, len(m.Indexes.Rows), 0)
		assert.Greater(t, len(m.Indexes.Columns), 0)

		@@ -247,17 +253,14 @@ func TestConstraints(t *testing.T) {

		c, _ := New(opts)

		t.Logf("constraints columns %v", co)
		t.Logf("constraints content %v", r)

Databases tree view #245

Databases tree view #245

Conversation

danvergara commented Nov 15, 2024 • edited Loading

Add a view to see databases and their tables

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccoVeille Nov 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danvergara commented Nov 17, 2024

ccoVeille commented Nov 17, 2024

ccoVeille commented Nov 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danvergara Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danvergara commented Nov 18, 2024

Arturomtz8 left a comment

Choose a reason for hiding this comment

danvergara commented Nov 15, 2024 •

edited

Loading

ccoVeille Nov 16, 2024 •

edited

Loading

danvergara Nov 17, 2024 •

edited

Loading